-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trusted entitlements: Add integration tests #1071
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1071 +/- ##
=======================================
Coverage 85.64% 85.64%
=======================================
Files 176 176
Lines 6247 6256 +9
Branches 859 861 +2
=======================================
+ Hits 5350 5358 +8
Misses 563 563
- Partials 334 335 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start 👏🏻 just a question about test setup to be able to test changing conditions (I still have to add tests like that in iOS too).
if (appConfig.forceSigningErrors) { | ||
warnLog("Forcing signing error for request with path: $urlPath") | ||
return VerificationResult.FAILED | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish there was a way to ensure this cannot run in production builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah honestly it's not great... With the server version of this I was thinking of a way by creating a new class that exists in each flavor (the integration test and the defaults flavors). The defaults flavor version wouldn't do anything and the integration test flavor would indicate the failure, however, we would still have to use that code here and it would complicate things more... I would really like to have what we have in iOS where we can make code compile only with certain flags but that's more difficult to architecture in Android and not sure if it's worth it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually having a file that's different on each flavor, wouldn't it be possible to at least detect the flavor at runtime?
So AppConfig
for example can't even allow this value being true
in the "release" flavor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! We actually can get the flavor from BuildConfig.FLAVOR
without extra files. We can use that to do what you said yeah. It would still have this code in the final bundle, but it would be another safety measure, I can do it like that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you're thinking of something better, but we could change AppConfig
so that some values can never return anything but the default depending on BuildConfig.FLAVOR
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the base PR: e16c410
@@ -16,6 +16,7 @@ class AppConfig( | |||
val dangerousSettings: DangerousSettings = DangerousSettings(autoSyncPurchases = true), | |||
// Should only be used for tests | |||
var forceServerErrors: Boolean = false, | |||
var forceSigningErrors: Boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to be overly cautious it would help to write a unit test to verify that this is false
by default, as a way to ensure production builds never ship with this on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the base PR in 371d747
|
||
@OptIn(ExperimentalPreviewRevenueCatPurchasesAPI::class) | ||
@RunWith(AndroidJUnit4::class) | ||
class TrustedEntitlementsInformationalFailureIntegrationTest : BasePurchasesIntegrationTest() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe TrustedEntitlementsInformationalModeIntegrationTest
? Since you might want to test successes too?
Oh I see you have a separate one. I'm usually not against splitting test classes, easier to maintain. But I think it might be weird because it would be really useful to add tests that verify the behavior when the signature goes from correct to invalid and vice-versa. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I added one in the success file, but it's true it doesn't fully fit... I was torn because I didn't want to make a huge test class and it's easier to setup the condition in the @Before
of the test, but well I think we don't have that many currently and we can indeed modify these things later in the integration test, so I will change it back to only one class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in dc6a1fa
@Test | ||
fun initialCustomerInfoIsVerified() { | ||
var receivedCustomerInfo: CustomerInfo? = null | ||
ensureBlockFinishes { latch -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't wait to see these with coroutines 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty straightforward and neat 💅 #1077
...enuecat/purchases/trustedentitlements/TrustedEntitlementsInformationalModeIntegrationTest.kt
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
fun canPurchaseProductFailingToVerifyStatus() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice 👍🏻
fdbc826
to
3d0190f
Compare
**This is an automatic release.** ### Bugfixes * Default customer info schema version to latest known by SDK (#1080) via Toni Rico (@tonidero) * Handle other diagnostics-related exceptions (#1076) via Toni Rico (@tonidero) * Return error in queryPurchases if error connecting to billing client (#1072) via Toni Rico (@tonidero) ### Other Changes * Fix offline entitlements integration tests (#1085) via Toni Rico (@tonidero) * Add defaultsRelease variant tests run configuration (#1074) via Toni Rico (@tonidero) * Compose sample app: move to gradle catalog (#1081) via Toni Rico (@tonidero) * Compose sample app: automate builds (#1082) via Toni Rico (@tonidero) * Compose sample app (#1056) via Toni Rico (@tonidero) * Migrate to Gradle version catalog (#1059) via Cesar de la Vega (@vegaro) * Trusted entitlements: Add logs with verification mode (#1067) via Toni Rico (@tonidero) * Sync pending purchases before getting customer info (#1073) via Toni Rico (@tonidero) * Refactor syncing pending transactions logic out of `Purchases` (#1058) via Toni Rico (@tonidero) * Refactor CustomerInfo listener and cache logic into CustomerInfoUpdater (#1052) via Toni Rico (@tonidero) * Trusted entitlements: Add integration tests (#1071) via Toni Rico (@tonidero) * Trusted entitlements: Add internal mechanism to force signing errors for tests (#1070) via Toni Rico (@tonidero) Co-authored-by: revenuecat-ops <[email protected]>
…tlementsInformationalModeIntegrationTest` (#1077) <!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### Checklist - [ ] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Motivation **Why is this change required? What problem does it solve?** Follow up from #1071 (comment) <!-- Please link to issues following this format: Resolves #999999 --> ### Description **Describe your changes in detail** - Adds `awaitCustomerInfo` / coroutines tests to `TrustedEntitlementsInformationalModeIntegrationTest` **Please describe in detail how you tested your changes** - Run integration tests locally ✅ cc @tonidero @vegaro @NachoSoto
…tlementsInformationalModeIntegrationTest` (#1077) via @pablo-guardiola (#1107) <!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### Checklist - [ ] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Motivation **Why is this change required? What problem does it solve?** Follow up from #1071 (comment) <!-- Please link to issues following this format: Resolves #999999 --> ### Description **Describe your changes in detail** - Adds `awaitCustomerInfo` / coroutines tests to `TrustedEntitlementsInformationalModeIntegrationTest` **Please describe in detail how you tested your changes** - Run integration tests locally ✅ Contributed by @pablo-guardiola Co-authored-by: pablo-guardiola <[email protected]>
Description
This adds integration tests to verify the basic behavior of the trusted entitlements feature.